Fix deserialization of unions #319
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Collapses the child elements into one, without class heirarchy, mixins, etc This is a replaying of 2670d11 (first implementation) and 9f5b95a (a bugfix) onto the new `main-v.0.7.0`, modified for the refactored upstream. This should bring `main-v.0.7.1` up to par with `main` for the features we implemented in our fork (dropping our `Unset` implementation for theirs)
Add support for generating setup.py and the initially generated one
|
@forest-benchling sure thing, I'll make sure to take a look at them before then. Can you rebase open PRs against main so it's easier to see what changed? Also since I've decided not to go forward with #297, if this really does depend on that is it possible to separate them? |
Yes, I will try. Sorry, this will take some time because so much has happened in the meantime that the merge conflicts are a mess 😬 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
NOTE: this has merge conflicts, because it's based off the
benchlingfork. I just wanted to get it up for review for visibility, I will change it to be based offtriaxteconce you have bandwidth to review it.Depends on #297
Deserialization of unions in
from_dictwas not working properly, because it failed to account for cases where the union property isNoneorUnset. This PR adds explicit checks forNoneorUnset.In addition, the type of the value to deserialize was previously set to
Any, which was masking type errors. In order to fix these type errors, I introduced a new concept ofcheck_type_for_constructfor each property, which allows the type to be checked before attempting deserialization.In the future, I think we should refine the
try/exceptpattern; it's not good practice to rely on a catch-allexceptwhen trying to deserialize each type within the union, because we don't know what exception was actually raised.